Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

implement api-level request size limit for oas (TT-11459) #6822

Merged
merged 6 commits into from
Jan 13, 2025
Merged

implement api-level request size limit for oas (TT-11459) #6822

merged 6 commits into from
Jan 13, 2025

Conversation

pvormste
Copy link
Contributor

@pvormste pvormste commented Jan 9, 2025 •
edited by github-actions bot
Loading

User description

TT-11459
Summary [OAS] Implement API-level request size limit
Type Story Story
Status In Dev
Points N/A
Labels -

This PR adds support for API-level request size limit for OAS.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

PR Type

Enhancement, Tests, Documentation

Description

  • Implemented API-level request size limit for OAS.

  • Added GlobalRequestSizeLimit struct and related methods.

  • Updated OAS schema and documentation for request size limit.

  • Added comprehensive tests for request size limit functionality.

Changes walkthrough 📝

Relevant files
Enhancement
api_definitions.go
Add support for request size limit in API definitions       

apidef/api_definitions.go

  • Added GlobalSizeLimitDisabled field to VersionInfo.
  • Enhanced API definition to support request size limit configuration.
    		• +1/-0     
    middleware.go
    Add request size limit handling in middleware                       

    apidef/oas/middleware.go

  • Introduced RequestSizeLimit field in Global struct.
  • Added methods to handle request size limit configuration.
  • Refactored version handling with helper functions.
    		• +88/-7   
    mw_request_size_limit.go
    Update middleware to respect request size limit                   

    gateway/mw_request_size_limit.go

    • Updated EnabledForSpec to check GlobalSizeLimitDisabled.
    		 +1/-1     
    x-tyk-api-gateway.json
    Update OAS schema for request size limit                                 

    apidef/oas/schema/x-tyk-api-gateway.json

  • Added X-Tyk-GlobalRequestSizeLimit definition.
  • Updated schema to include requestSizeLimit field.
    		• +18/-0   
    Tests
    middleware_test.go
    Add tests for request size limit functionality                     

    apidef/oas/middleware_test.go

  • Added tests for GlobalRequestSizeLimit functionality.
  • Covered both Fill and ExtractTo methods.
    		• +144/-0 
    oas_test.go
    Update OAS tests for request size limit                                   

    apidef/oas/oas_test.go

    • Updated tests to include GlobalSizeLimitDisabled field.
    		 +1/-1     
    mw_request_size_limit_test.go
    Add middleware tests for request size limit                           

    gateway/mw_request_size_limit_test.go

    • Added tests for EnabledForSpec with various configurations.
    		 +47/-0   
    Documentation
    apidef-oas.md
    Update documentation for request size limit                           

    docs/dev/apidef-oas.md

  • Documented VersionData handling for request size limit.
  • Added examples for Fill and ExtractTo methods.
    		• +19/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @buger
    Copy link
    Member

    buger commented Jan 9, 2025

    I'm a bot and I 👍 this PR title. 🤖

    @github-actions GitHub Actions
    Copy link
    Contributor

    github-actions bot commented Jan 9, 2025 •
    edited
    Loading

    API Changes

    --- prev.txt	2025-01-13 10:00:34.069733824 +0000
+++ current.txt	2025-01-13 10:00:29.589769935 +0000
@@ -1457,6 +1457,7 @@
 	GlobalResponseHeadersDisabled bool              `bson:"global_response_headers_disabled" json:"global_response_headers_disabled"`
 	IgnoreEndpointCase            bool              `bson:"ignore_endpoint_case" json:"ignore_endpoint_case"`
 	GlobalSizeLimit               int64             `bson:"global_size_limit" json:"global_size_limit"`
+	GlobalSizeLimitDisabled       bool              `bson:"global_size_limit_disabled" json:"global_size_limit_disabled"`
 	OverrideTarget                string            `bson:"override_target" json:"override_target"`
 }
 
@@ -2729,6 +2730,9 @@
 
 	// TrafficLogs contains the configurations related to API level log analytics.
 	TrafficLogs *TrafficLogs `bson:"trafficLogs,omitempty" json:"trafficLogs,omitempty"`
+
+	// RequestSizeLimit contains the configuration related to limiting the global request size.
+	RequestSizeLimit *GlobalRequestSizeLimit `bson:"requestSizeLimit,omitempty" json:"requestSizeLimit,omitempty"`
 }
     Global contains configuration that affects the whole API (all endpoints).
 
@@ -2745,6 +2749,23 @@
     ensures backwards compatibility and proper handling of the deprecated fields
     during the migration process.
 
+type GlobalRequestSizeLimit struct {
+	// Enabled activates the Request Size Limit.
+	// Tyk classic API definition: `version_data.versions..global_size_limit_disabled`.
+	Enabled bool `bson:"enabled" json:"enabled"`
+	// Value contains the value of the request size limit.
+	// Tyk classic API definition: `version_data.versions..global_size_limit`.
+	Value int64 `bson:"value" json:"value"`
+}
+    GlobalRequestSizeLimit holds configuration about the global limits for
+    request sizes.
+
+func (g *GlobalRequestSizeLimit) ExtractTo(api *apidef.APIDefinition)
+    ExtractTo extracts *GlobalRequestSizeLimit into *apidef.APIDefinition.
+
+func (g *GlobalRequestSizeLimit) Fill(api apidef.APIDefinition)
+    Fill fills *GlobalRequestSizeLimit from apidef.APIDefinition.
+
 type HMAC struct {
 	// Enabled activates the HMAC authentication mode.
 	// Tyk classic API definition: `enable_signature_checking`

    @github-actions GitHub Actions
    Copy link
    Contributor

    github-actions bot commented Jan 9, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Issue

    The fillRequestSizeLimit function in the Global struct assumes that the RequestSizeLimit field is always initialized correctly. Ensure that edge cases where api.VersionData.Versions is nil or improperly structured are handled gracefully.

    func (g *Global) fillRequestSizeLimit(api apidef.APIDefinition) {
	if g.RequestSizeLimit == nil {
		g.RequestSizeLimit = &GlobalRequestSizeLimit{}
	}

	g.RequestSizeLimit.Fill(api)
	if ShouldOmit(g.RequestSizeLimit) {
		g.RequestSizeLimit = nil
	}
}
    Logic Validation

    The EnabledForSpec function now includes a condition for GlobalSizeLimitDisabled. Ensure that this logic correctly handles all scenarios, especially when GlobalSizeLimit is set to 0 or disabled.

    func (t *RequestSizeLimitMiddleware) EnabledForSpec() bool {
	for _, version := range t.Spec.VersionData.Versions {
		if len(version.ExtendedPaths.SizeLimit) > 0 ||
			(!version.GlobalSizeLimitDisabled && version.GlobalSizeLimit > 0) {
			return true
		}
	}
	return false

    @github-actions GitHub Actions
    Copy link
    Contributor

    github-actions bot commented Jan 9, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Add test cases for nil VersionData.Versions in TestGlobalRequestSizeLimit

    Include test cases for scenarios where api.VersionData.Versions is nil in the
    TestGlobalRequestSizeLimit test suite to ensure robustness.

    apidef/oas/middleware_test.go [1044]

     testcases := []struct {
     title    string
     input    apidef.APIDefinition
     expected *GlobalRequestSizeLimit
 }{
     {
         title:    "no versions",
         input:    apidef.APIDefinition{},
         expected: nil,
     },
+    {
+        title:    "nil versions map",
+        input:    apidef.APIDefinition{VersionData: apidef.VersionData{Versions: nil}},
+        expected: nil,
+    },
     ...
 }
    Suggestion importance[1-10]: 7

    Why: Including a test case for nil VersionData.Versions enhances the robustness of the test suite by covering an edge case that could occur in real-world scenarios.

    		7
    Ensure GlobalSizeLimit is validated as non-negative in EnabledForSpec

    Validate that version.GlobalSizeLimit is non-negative in EnabledForSpec to prevent
    potential logical errors or misuse.

    gateway/mw_request_size_limit.go [42]

    -(!version.GlobalSizeLimitDisabled && version.GlobalSizeLimit > 0)
+(!version.GlobalSizeLimitDisabled && version.GlobalSizeLimit >= 0)
    Suggestion importance[1-10]: 2

    Why: The suggestion to validate GlobalSizeLimit as non-negative is redundant since the condition already ensures it is greater than 0. This adds minimal value to the existing logic.

    		2
    Possible issue
    Add a nil check for api.VersionData.Versions before calling fillRequestSizeLimit

    Ensure that the fillRequestSizeLimit function properly handles cases where
    api.VersionData.Versions is nil to avoid potential nil pointer dereferences.

    apidef/oas/middleware.go [234]

    -g.fillRequestSizeLimit(api)
+if api.VersionData.Versions != nil {
+    g.fillRequestSizeLimit(api)
+}
    Suggestion importance[1-10]: 3

    Why: Adding a nil check for api.VersionData.Versions could prevent potential nil pointer dereferences, but the PR already includes helper functions like requireMainVersion to handle such cases. This makes the suggestion less impactful.

    		3

    jeffy-mathew
    jeffy-mathew approved these changes Jan 10, 2025
    View reviewed changes
    Copy link
    Contributor

    @jeffy-mathew jeffy-mathew left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    lgtm

    @pvormste pvormste force-pushed the feat/TT-11459/api-level-request-size-limit branch from 48303e3 to 9f4752d Compare January 13, 2025 09:59
    @pvormste pvormste enabled auto-merge (squash) January 13, 2025 10:00
    @sonarqubecloud SonarQubeCloud
    Copy link

    Quality Gate Failed Quality Gate failed

    Failed conditions
    1 Security Hotspot

    See analysis details on SonarQube Cloud

    @pvormste pvormste merged commit 93e3151 into master Jan 13, 2025
    29 of 41 checks passed
    @pvormste pvormste deleted the feat/TT-11459/api-level-request-size-limit branch January 13, 2025 10:22
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @jeffy-mathew jeffy-mathew jeffy-mathew approved these changes

    @kofoworola kofoworola Awaiting requested review from kofoworola

    @buraksezer buraksezer Awaiting requested review from buraksezer

    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 4 Story Tyk Product Development
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    3 participants
    @pvormste @buger @jeffy-mathew